src/rpc/quorums.cpp (1)

1311-1330: Validate input object structure in ParseCheckpointFromRPC.

The helper function directly accesses keys like checkpointObj["block_hash"] and checkpointObj["chainlock_quorums"] without first verifying they exist. If a user provides a malformed checkpoint object missing required keys, this will throw a less informative exception.

Consider adding existence checks or using .find() with appropriate error messages for better RPC error handling.

♻️ Suggested improvement

 static llmq::QuorumCheckpoint ParseCheckpointFromRPC(const UniValue& checkpointObj)
 {
+    if (!checkpointObj.exists("block_hash") || !checkpointObj.exists("height") || 
+        !checkpointObj.exists("chainlock_quorums")) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Checkpoint must contain block_hash, height, and chainlock_quorums");
+    }
+
     llmq::QuorumCheckpoint checkpoint;
     checkpoint.blockHash = ParseHashV(checkpointObj["block_hash"], "block_hash");
     // ... rest unchanged

test/functional/feature_quorum_proof_chain.py (2)

49-78: Consider catching specific JSONRPCException instead of broad Exception.

The broad except Exception catches can mask unexpected failures. For RPC error handling in tests, catching JSONRPCException specifically would be more precise and help detect actual test failures vs expected "not found" responses.

♻️ Suggested improvement

+from test_framework.authproxy import JSONRPCException
+
 # In test_chainlock_index:
         for h in range(tip_height, 200, -1):
             try:
                 cl_info = self.nodes[0].getchainlockbyheight(h)
                 # ... success handling
-            except Exception:
+            except JSONRPCException:
                 continue

---

115-142: Consider removing or using the build_checkpoint helper.

The build_checkpoint method is defined but never called in the test. If it's intended for future use with getquorumproofchain/verifyquorumproofchain tests, consider either:

1. Adding tests that exercise these RPCs using this helper, or
2. Adding a TODO comment explaining the intended future use

Currently, the test only covers getchainlockbyheight but not the proof chain generation/verification RPCs.

Would you like me to help draft additional test cases for getquorumproofchain and verifyquorumproofchain RPCs?

src/test/quorum_proofs_tests.cpp (1)

199-224: Consider adding FromJson roundtrip verification.

The test verifies ToJson output structure but doesn't complete the roundtrip by parsing with FromJson. Consider adding a full roundtrip test to ensure JSON serialization is bidirectional.

💡 Suggested enhancement

     BOOST_CHECK_EQUAL(json["height"].getInt<int>(), 1000);
+
+    // Verify FromJson roundtrip
+    llmq::QuorumCheckpoint parsed = llmq::QuorumCheckpoint::FromJson(json);
+    BOOST_CHECK(parsed.blockHash == checkpoint.blockHash);
+    BOOST_CHECK_EQUAL(parsed.height, checkpoint.height);
+    BOOST_CHECK_EQUAL(parsed.chainlockQuorums.size(), checkpoint.chainlockQuorums.size());
 }

src/llmq/quorumproofs.cpp (2)

158-178: Consider adding JSON field existence validation.

FromJson directly accesses JSON fields without checking existence first. If a caller provides malformed JSON missing required fields, the error message may be unclear. Consider validating field presence.

💡 Suggested improvement

 QuorumCheckpoint QuorumCheckpoint::FromJson(const UniValue& obj)
 {
     QuorumCheckpoint checkpoint;
 
+    if (!obj.exists("blockHash") || !obj.exists("height") || !obj.exists("chainlockQuorums")) {
+        throw std::runtime_error("Missing required fields in checkpoint JSON");
+    }
+
     checkpoint.blockHash = uint256S(obj["blockHash"].get_str());
     checkpoint.height = obj["height"].getInt<int32_t>();

---

230-253: Consider consolidating duplicated merkle proof verification logic.

The static VerifyMerkleProof function duplicates the logic in QuorumMerkleProof::Verify. Consider having one call the other to reduce code duplication.

💡 Suggested refactor

 static bool VerifyMerkleProof(const uint256& leafHash,
                                const std::vector<uint256>& merklePath,
                                const std::vector<bool>& merklePathSide,
                                const uint256& expectedRoot)
 {
-    if (merklePath.size() != merklePathSide.size()) {
-        return false;
-    }
-
-    if (merklePath.size() > MAX_MERKLE_PATH_LENGTH) {
-        return false;
-    }
-
-    uint256 current = leafHash;
-    for (size_t i = 0; i < merklePath.size(); ++i) {
-        if (merklePathSide[i]) {
-            current = Hash(current, merklePath[i]);
-        } else {
-            current = Hash(merklePath[i], current);
-        }
-    }
-
-    return current == expectedRoot;
+    QuorumMerkleProof proof;
+    proof.merklePath = merklePath;
+    proof.merklePathSide = merklePathSide;
+    return proof.Verify(leafHash, expectedRoot);
 }

src/llmq/quorumproofs.h (1)

209-222: Move DB_CHAINLOCK_BY_HEIGHT to an anonymous namespace or make it inline.

The static const std::string in a header creates a separate copy in each translation unit that includes this header. For a string constant used as a DB key, this wastes memory. Consider using inline constexpr (C++17) or moving to an anonymous namespace in the .cpp file.

💡 Suggested fix

Move to the .cpp file within an anonymous namespace:

// In quorumproofs.cpp
namespace {
const std::string DB_CHAINLOCK_BY_HEIGHT = "q_clh";
} // anonymous namespace

Or if it must remain in the header (C++17):

-static const std::string DB_CHAINLOCK_BY_HEIGHT = "q_clh";
+inline constexpr std::string_view DB_CHAINLOCK_BY_HEIGHT = "q_clh";

src/llmq/quorumproofs.cpp (1)

496-499: Chainlock block lookup can return nullptr when the chainlock height is ahead of the base block.

Line 496-499 uses GetAncestor(...), which only walks backward; for chainlock heights greater than the quorum base block height this yields nullptr. Prefer looking up by height on active_chain (as already flagged).

src/llmq/quorumproofs.cpp (1)

59-83: Avoid duplicate merkle-proof verification logic.

Line 59-83 and Line 231-254 implement the same hashing loop/DoS checks. Consider delegating to a single helper to prevent drift.

Also applies to: 231-254

test/functional/feature_quorum_proof_chain.py (1)

55-76: Avoid swallowing unexpected RPC errors.

Bare except Exception masks real failures in the scan loops; catching JSONRPCException keeps intent while preserving unexpected errors.

♻️ Suggested refinement (apply similarly to other loops)

-from test_framework.util import assert_equal, assert_raises_rpc_error
+from test_framework.util import assert_equal, assert_raises_rpc_error
+from test_framework.authproxy import JSONRPCException
@@
-            except Exception:
-                continue
+            except JSONRPCException as e:
+                if e.error.get("code") != -5:
+                    raise
+                self.log.debug(f"Height {h} not chainlocked yet: {e}")
+                continue
@@
-            except Exception:
-                continue
+            except JSONRPCException as e:
+                if e.error.get("code") != -5:
+                    raise
+                self.log.debug(f"Height {h} not chainlocked yet: {e}")
+                continue
@@
-        try:
-            cl_quorums = self.nodes[0].quorum("list", llmq_type)
-        except Exception:
-            # If quorum list fails, try with different type
-            cl_quorums = []
+        try:
+            cl_quorums = self.nodes[0].quorum("list", llmq_type)
+        except JSONRPCException as e:
+            self.log.debug(f"quorum list failed for type {llmq_type}: {e}")
+            cl_quorums = []
@@
-            except Exception:
-                continue
+            except JSONRPCException as e:
+                self.log.debug(f"quorum info failed for {qhash}: {e}")
+                continue

Also applies to: 86-95, 120-136

src/llmq/quorumproofs.cpp (1)

851-910: Header continuity check conflicts with proof layout; chainlocks aren’t anchored to headers.

The headers here are the commitment-mined blocks, which are typically not consecutive, so the strict prevBlockHash chain will reject multi-step proofs. Also, the chainlock signature isn’t tied to any header hash, so an unrelated header chain could still satisfy the merkle proofs. Consider either including intermediate headers up to the chainlock block, or anchoring each proof by requiring chainlock.blockHash == header.GetHash() and adjusting generation/verification accordingly.

src/llmq/blockprocessor.cpp (1)

167-211: Consider consolidating the merkle-path helper.

This helper is duplicated in src/llmq/quorumproofs.cpp. Extracting a single shared implementation will reduce the risk of subtle divergence later.

src/llmq/quorumproofs.h (1)

8-18: Make the header self-contained for UniValue / std::map / std::string.

These types are used directly but the header doesn’t include their declarations. If they aren’t pulled transitively, this header won’t compile on its own. Consider adding explicit includes (or a UniValue forward declaration if you prefer to keep the include light).

🛠️ Proposed fix

 `#include` <llmq/types.h>
 `#include` <primitives/block.h>
 `#include` <primitives/transaction.h>
 `#include` <serialize.h>
 `#include` <uint256.h>
+#include <univalue.h>
 
+#include <map>
+#include <string>
 `#include` <set>
 `#include` <vector>

src/llmq/quorumproofs.cpp (3)

1-1: Nit: Copyright year should be 2026.

As noted in a past review comment, the current date is January 2026, but the copyright header says 2025.

🔧 Suggested fix

-// Copyright (c) 2025 The Dash Core developers
+// Copyright (c) 2026 The Dash Core developers

---

294-338: Consider extracting BuildMerkleProofPath to a shared utility.

This function is duplicated verbatim from src/llmq/blockprocessor.cpp (lines 170-210). Consider moving it to a shared header (e.g., src/consensus/merkle.h or a new src/llmq/merkle_utils.h) to avoid duplication and ensure both implementations stay in sync.

---

1156-1160: Progress percentage calculation is a rough estimate.

The progress calculation indexed_count / 10 is a rough estimate that may not reflect actual progress. For example, if there are 500 total quorums, progress would cap at ~50% before completing. Consider tracking the total count upfront for accurate progress display, or document that this is an approximate indicator.

test/functional/feature_quorum_proof_chain.py (3)

114-141: build_checkpoint helper is defined but never called.

The build_checkpoint method is implemented but not used in run_test. If this is intended for future test expansion (e.g., testing getquorumproofchain/verifyquorumproofchain), consider adding a TODO comment. Otherwise, it could be removed to avoid dead code.

---

55-77: Consider catching specific JSONRPCException instead of bare Exception.

The try-except-continue pattern is common in test iteration, but catching a specific exception type would be more precise and avoid masking unexpected errors:

🔧 Suggested improvement

+from test_framework.authproxy import JSONRPCException
 ...
         for h in range(tip_height, 0, -1):
             try:
                 cl_info = self.nodes[0].getchainlockbyheight(h)
                 ...
                 return
-            except Exception:
+            except JSONRPCException:
                 continue

---

43-46: Consider adding tests for getquorumproofchain and verifyquorumproofchain RPCs.

The functional test covers getchainlockbyheight but not the proof chain generation/verification RPCs. The build_checkpoint helper suggests these were planned. Adding coverage would validate the end-to-end proof chain workflow.

Would you like me to help draft additional test cases for these RPCs?

src/rpc/quorums.cpp (1)

1376-1381: Inconsistent LLMQ type parsing between RPCs.

getquorumproofchain uses getInt<int>() (line 1377) while verifyquorumproofchain uses getInt<uint8_t>() (line 1462) for parsing the LLMQ type. Since LLMQType is enum class LLMQType : uint8_t, consider using uint8_t consistently:

🔧 Suggested fix

-    const Consensus::LLMQType targetType = static_cast<Consensus::LLMQType>(request.params[2].getInt<int>());
+    const Consensus::LLMQType targetType = static_cast<Consensus::LLMQType>(request.params[2].getInt<uint8_t>());

src/llmq/quorumproofs.cpp (1)

759-772: Bind the header to the chainlock’s signed block hash.

Right now the proof can pair a valid chainlock signature for block A with an unrelated header/merkle root for block B. Validate the header hash matches chainlock.blockHash before merkle proof checks.

🛠️ Suggested guard

         const CBlockHeader& header = proof.headers[proofIdx];
+        if (header.GetHash() != chainlock.blockHash) {
+            result.error = strprintf("Header does not match chainlock block hash in proof %d", proofIdx);
+            return result;
+        }

src/llmq/blockprocessor.cpp (1)

606-634: Fast-path hash assumes SER_DISK == SER_GETHASH — add a guard/test.

If CFinalCommitment serialization ever diverges, this path would silently compute a different hash than SerializeHash and poison proofs. Consider a debug-only assertion or a unit test to lock the invariant. If you use the assert, add <cassert> if it's not already included.

🛠️ Suggested debug guard

-            return Hash(MakeByteSpan(ssValue).first(ssValue.size() - 32));
+            const uint256 fast_hash = Hash(MakeByteSpan(ssValue).first(ssValue.size() - 32));
+#ifdef DEBUG
+            auto [commitment, _] = GetMinedCommitment(llmqType, quorumHash);
+            if (!commitment.IsNull()) {
+                assert(fast_hash == ::SerializeHash(commitment));
+            }
+#endif
+            return fast_hash;